-
-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow mermaid to be configured via some query params #1413
Conversation
I think we need to establish a convention for configuration using nested names. We should also decide how we can infer the value type (array, boolean, number...). Related: #1410 (comment) Configuration can be sent ad HTTP headers. In theory, we can use
Reference: https://www.rfc-editor.org/rfc/rfc7230#section-3.2 But some providers, for instance Cloudflare, apply restrictions:
So, it might be safer to use a mix of snake_case and kebab-case. For instance:
Please note that header name are case insensitive.
As mentioned in #1410 I'm leaning toward HTTP headers (rather than query parameters) to send diagram options from the gateway server to a companion container. I think it has less restrictions than a query parameters (especially regarding the URL length). |
yeah, But the case insensitiveness of Headers can be problematic. We will need to keep track of all mermaid options and make case insensitive matches to figure out which property is which. This might become more problematic if mermaid expects a case sensitive value. Any opinions on this situation? |
I don't think that's required, we can use the following transformations:
|
|
Wait, I don't think this will work with how things are now. RIght now, I am dependendant on the mkdocs kroki plugin (https://github.com/AVATEAM-IT-SYSTEMHAUS/mkdocs-kroki-plugin) cause my only usecase is with mkdocs at this point. And although that plugin provides a GET and POST methods, they provide no way to send any info via headers. Moreover, imagine that plugin could do that, it would still not solve the problem of generating a diagram with a theme given at runtime. Right now, the diagram is fetched via a I didn't think of this issue before and was thinking about headers being a good idea but at this point I don't see a way to pass parameters to mermaid without using query params or changing a lot of tools around this ecosystem. |
In AsciiDoc, we are using block attributes to send diagram options.
Sorry I wasn't clear.
You don't, the gateway server will receive diagram options as query parameters, as attributes in the JSON body or as HTTP headers and pass them as HTTP headers to the Mermaid companion container. |
To give more context, I want to document/specify the HTTP endpoint of the companion containers so others can contribute additional companion containers. GET http://my.kroki/foo/svg/diagram?some-option=bar |
Let me see if I understand this correctly. The Java based Gatewat Server (which has the Mermaid.js service) already implement the handling of query parameters, body or headers and handles them by stripping all query parameters if present, body if present and makes a request with only Headers to the Js + micro based mermaid server. Only the mermaid server doesn't handle headers yet. So, I won't need to make any updates to the gateway server for instance? |
Just checked, seems like the gateway server is not modifying the headers at all right now. But I get your train of thought. This is going to be a bit of a change but I will see what I can do. |
Something I noticed, out of all the diagram types that options can be passed to as in https://docs.kroki.io/kroki/setup/diagram-options/, only Line 117 in 7347d8b
Are you still sure that the gateway should be passing headers to underlying Moreover, the |
You're correct, currently, the gateway server makes a request to the Mermaid companion service/container using query parameters:
So we will need to change the code to make a request with HTTP headers... but I can take care of that part 😉
I'm fine with implementing this feature using query parameters and do the change I've mentioned in another pull request. My point was that we need to make sure that we won't have any issue when migrating from query parameters to HTTP headers (more specifically on the naming convention we've established). |
Yeah, and if we are doing it, we should implement something similar to So, I will plan for working on this tomorrow. Will this PR be fine or now so that you merge this and we can make the move to Headers in a new PR, or should I make the Header implementations in this PR itself. Note that header implementation means changing the gateway server too. Honestly I can do that too. Its just that I need to know the direction I should follow. |
I would prefer to migrate from query parameters to HTTP headers in a new pull request.
|
Pushed a new commit to the PR. This commit would handle boolean, number and array values. But this commit will still not work correctly because the gateway is lowercasing all query parameters before forwarding them. From what I see the gateway server is lowercasing all query parameters in The way I prefer things to work in the long run: Options can be passed via query parameters and headers to the gateway server. The gateway server converts the both of them into headers only. Query params can be passed as property names with dot notation and case preserved. For eg Arrays can be passed by using array notation. For eg. The above 2 can be passed as headers in a case insensitive way as such
The PR in its current state cannot handle query parameters are above. The gateway server converts Let me know what you think. |
I guess we can support both. I like the naming consistency (i.e. we can use
the same diagram option name no matter how we send it).
If we replace `_` by `.` and dash-separated `-` words by camelCase we can
safely send: `er.useMaxWidth=true` or `er_use-MAX-Width=true` (which will
be transformed to `er.useMaxWidth=true`).
If the name contains `_` or `-` then we use lowercase and apply
transformations. Otherwise, we use the name as-is.
Le lun. 2 janv. 2023, 11:14, Sayak Mukhopadhyay ***@***.***>
a écrit :
… Pushed a new commit to the PR. This commit would handle boolean, number
and array values. But this commit will still not work correctly because the
gateway is lowercasing all query parameters before forwarding them.
From what I see the gateway server is lowercasing all query parameters in
DiagramHandler. From what I know, URLs are case sensitive, at least in
the path and query params. Lowercasing headers I understand, but lower
casing query params means that I would need to send er.use_max_width in
query params too. That mean losing 1 character that could have been data of
the 2048 characters the URL can be. Moreover, Delegator.delegate() is
using the options (that has both header based and query parameter based
configs) and putting them as query parameters for the next container to use.
*The way I prefer things to work in the long run:*
Options can be passed via query parameters and headers to the gateway
server. The gateway server converts the both of them into headers only.
Query params can be passed as property names with dot notation and case
preserved. For eg
https://kroki.io/mermaid/svg/eNpLL0osyFDwCeJSAALH6IDEotS8EgXDWAVdXTsFp2jnjMycFCAXIg0WdIYKGiELukAFjRWgJkBlXcCyrlBZE2RBN6igKUgQADblIZ4?theme=dark&er.useMaxWidth=true
Arrays can be passed by using array notation. For eg.
https://kroki.io/mermaid/svg/eNpLL0osyFDwCeJSAALH6IDEotS8EgXDWAVdXTsFp2jnjMycFCAXIg0WdIYKGiELukAFjRWgJkBlXcCyrlBZE2RBN6igKUgQADblIZ4?er.foo=[bar,tar,jar]
The above 2 can be passed as headers in a case insensitive way as such
'kroki-diagram-options-theme': 'dark',
'kroki-diagram-options-er_use-max-width': 'true',
'kroki-diagram-options-er_foo': '[bar,tar,jar]'
The PR in its current state cannot handle query parameters are above. The
gateway server converts er.useMaxWidth to er.usemaxwidth. As such the
only way I can make the current PR work is if I start sending the query
parameters as er.use_max_width. Then I would need to reconvert the
underscores in camel case in the mermaid container. But as I mentioned
before I would prefer to be able to send correct casing in the URL itself.
Let me know what you think.
—
Reply to this email directly, view it on GitHub
<#1413 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACRLXF4OTNJF2TFPWI5QF3WQKS63ANCNFSM6AAAAAATM7RGN4>
.
You are receiving this because you commented.
Message ID: ***@***.***>
|
7cc8093
to
66c072b
Compare
I have pushed a new commit. This commit handled mermaid configuration properties as such: Or as Headers
|
…ons-er_use-max-width` where the mermaid option is `er.useMaxWidth`
…ssed through Headers
c530cf6
to
b9a2a04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you contribution! It looks good 👍🏻
I will try to add a few tests to catch potential regression when we will move from query parameters to HTTP headers.
Anyway, great work 🙌🏻
@SayakMukhopadhyay, that link no longer works? (Why does a 400 error show OK?) |
@DaveJarvis The link I entered was an example of how this PR will behave once deployed. I am not sure if this PR has been deployed yet. @Mogztter would be able to tell that. |
No, I didn't release/deploy a new version yet.
Are you suggestion that it was working before?
400 could be a syntax error but we don't know precisely. Arguably, we could return something else than OK... I wasn't sure about Bad Request because technically the query is valid even though the diagram syntax is invalid. |
Would 422 be appropriate?
https://www.rfc-editor.org/rfc/rfc4918#section-11.2
No, I misunderstood; didn't realize that completing the PR didn't result in deployment. |
@Mogztter Before my PR, the gateway server passes query parameters in the URL to the mermaid container. But the mermaid container makes a regex check on the URL to see if it ends with "mermaid". Sending query parameters fails this check and hence the error. |
Thanks for taking the time to investigate 👌 |
@SayakMukhopadhyay @ggrossetie |
This PR tries to implement the ability of configuring some options of mermaid using query params of the GET request.
This will only handle properties that have string values and won't handle anything else at the moment. This is mostly because I am not sure how to send configuration values of
flowchart.useMaxWidth
. If there is some clarity on that I can implement that too.Fixes #1326